Stokes_Constrained: unlock parallel (remove over-conservative serial guard)#240
Merged
Conversation
…guard) The in-saddle multiplier free-slip solver was guarded serial-only, but the interior-multiplier reduction (_constrain_interior_multipliers_in_section) is rank-local section surgery: it uses the distributed boundary-label IS and iterates the local chart, so the global system — and hence the velocity solve and the gauge-invariant boundary traction — are partition-independent. Validated bit-identical at np=1/2/4 (velocity L2 and mean-stripped boundary topography) for both isotropic and transverse-isotropic rheology. On enclosed problems the raw multiplier h carries the [p,lambda] gauge constant (the solver lands on a partition-dependent representative). topography() gains a reference="mean" option to subtract the boundary mean for a reproducible, gauge-fixed readout; the default (reference=None) is unchanged (raw multiplier), which is correct for problems with no gauge freedom (e.g. an open boundary). - src/underworld3/systems/solvers.py: remove serial guard; topography(reference=). - tests/parallel/test_1063_constrained_freeslip_parallel.py: np>=2 regression. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR enables running SNES_Stokes_Constrained (the in-saddle Lagrange-multiplier free-slip Stokes solver) under MPI by removing the serial-only guard, and adds a new topography(reference=...) option intended to provide a reproducible (gauge-fixed) topography readout for enclosed problems. It also introduces a parallel regression test to validate partition-independence against stored serial “golden” diagnostics.
Changes:
- Remove the
uw.mpi.size > 1NotImplementedErrorguard fromSNES_Stokes_Constrained.add_constraint_bc. - Add
topography(..., reference=None|"mean"), with"mean"subtracting the boundary-mean multiplier viaBdIntegralreductions. - Add a new MPI regression test (
min_size=2) for isotropic and transverse-isotropic cases.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/underworld3/systems/solvers.py |
Removes the MPI guard and adds gauge-fixing support to topography(reference=...). |
tests/parallel/test_1063_constrained_freeslip_parallel.py |
Adds a new parallel regression test comparing MPI results to stored serial golden diagnostics. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+2244
to
+2248
| problems pass ``reference="mean"`` to subtract the boundary mean and obtain | ||
| a gauge-fixed, partition-independent topography. The default | ||
| (``reference=None``) returns the raw multiplier — correct for problems with | ||
| **no** gauge freedom (e.g. an open boundary), where the mean of :math:`h` is | ||
| the physical mean traction and must NOT be removed. |
Comment on lines
+9
to
+11
| * the velocity L2 norm (``∫ v·v``) is bit-identical serial vs parallel, and | ||
| * the MEAN-STRIPPED boundary topography (``∫(h - h̄)² `` on the constrained | ||
| boundary) is bit-identical serial vs parallel. |
Comment on lines
+80
to
+81
| """The parallel solve must reproduce the serial reference: velocity bit- | ||
| identical, and the gauge-fixed (mean-stripped) topography bit-identical.""" |
Comment on lines
+69
to
+75
| blen = float(uw.maths.BdIntegral( | ||
| mesh=mesh, fn=sympy.Integer(1), boundary="Upper").evaluate()) | ||
| hbar = float(uw.maths.BdIntegral( | ||
| mesh=mesh, fn=h.sym[0], boundary="Upper").evaluate()) / blen | ||
| topo = float(np.sqrt(uw.maths.BdIntegral( | ||
| mesh=mesh, fn=(h.sym[0] - hbar) ** 2, boundary="Upper").evaluate())) | ||
| return L2, topo |
…, wording - topography() docstring: note that reference="mean" runs collective BdIntegral reductions (must be called on all ranks); reference=None is a pure accessor. - parallel test: compute the gauge-fixed topography via solver.topography(boundary, reference="mean") so the new code path is covered. - test docstring: replace "bit-identical" with "tight tolerance" (the residual difference is the parallel reduction order, not the solver); np.isclose tolerances are intentional. Underworld development team with AI support from Claude Code
Member
Author
|
Addressed the Copilot review in a510445:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes the serial-only guard on
SNES_Stokes_Constrained(the in-saddle Lagrange-multiplier free-slip solver). It is parallel-safe; the guard was over-conservative.Why it was guarded, and why that was unnecessary
The multiplier is carried as a full-domain field whose interior DOFs are pinned away by
_constrain_interior_multipliers_in_section. That reduction is rank-local section surgery — it uses the distributed boundary-labelIS(getStratumIS) and iterates the local chart, constraining DOFs in the localPetscSectionper rank. So the assembled global system is partition-independent; there is no single-rank enumeration to MPI-decompose.Validation
Bit-identical serial-vs-parallel (np=1/2/4), isotropic and transverse-isotropic, on the enclosed free-slip annulus:
L2(v)∫(v·n)²∫(h−h̄)²BdIntegral(fn=1)(boundary length) is also bit-identical, confirming the surface integrator is parallel-safe. New regression test:tests/parallel/test_1063_constrained_freeslip_parallel.py(mpi(min_size=2)).The gauge constant
On an enclosed problem the raw multiplier
his determined only up to the[p,λ]gauge constant, and the solver lands on a partition-dependent representative of it (the velocity and the deviation ofhare unaffected).topography()gains areference="mean"option to subtract the boundary mean for a reproducible, gauge-fixed readout. The default is unchanged (reference=None, raw multiplier) — correct for problems with no gauge freedom (e.g. an open boundary), where the mean ofhis the physical mean traction.Changes
src/underworld3/systems/solvers.py: remove the serial guard; addtopography(reference=).tests/parallel/test_1063_constrained_freeslip_parallel.py: new np≥2 regression.Tests
test_1061,test_1062): 16 passed (no regression).Underworld development team with AI support from Claude Code